Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Avoid allocating the --help message #12243

Merged
merged 1 commit into from
Dec 16, 2024

Conversation

Integral-Tech
Copy link
Contributor

@Integral-Tech Integral-Tech commented Dec 13, 2024

  • Although format! macro is very powerful, it introduces overhead of memory allocation on the heap compared to &str. Therefore, when format! macro is unnecessary, we should avoid using it.

@Integral-Tech Integral-Tech changed the title refactor: replace format! with concat! or format_args! when unnecessary refactor: replace format! with concat! or print! when unnecessary Dec 14, 2024
Copy link
Contributor

@TornaxO7 TornaxO7 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In my opinion, this isn't really needed.
Yes, it's true, that format uses the heap, but performance wise it's neglectable.
I would prefer readability over "micro-performance-improvements" in this case.

helix-loader/build.rs Outdated Show resolved Hide resolved
@Integral-Tech Integral-Tech changed the title refactor: replace format! with concat! or print! when unnecessary refactor: replace format! with print! when String construction is unnecessary Dec 14, 2024
@Integral-Tech
Copy link
Contributor Author

@TornaxO7 I have just reverted the changes in helix-loader/build.rs.

@TornaxO7
Copy link
Contributor

Eh, I'm not a maintainer, so you needn't see my review as "mandatory". Those are just my two cents :>

@the-mikedavis
Copy link
Member

Yeah generally there's a tradeoff in refactoring for a performance gain. We typically try to avoid refactors unless they bring some concrete benefits. It'd be nice to not allocate this non-trivially-small string - previously we allocated it even when not printing the help message - and this block barely every changes so I'll merge this.

@the-mikedavis the-mikedavis changed the title refactor: replace format! with print! when String construction is unnecessary Avoid allocating the --help message Dec 16, 2024
@the-mikedavis the-mikedavis merged commit 250d9fa into helix-editor:master Dec 16, 2024
6 checks passed
@Integral-Tech Integral-Tech deleted the format-concat branch December 16, 2024 17:22
CedricMeu pushed a commit to CedricMeu/helix that referenced this pull request Jan 2, 2025
GladkihEgor pushed a commit to GladkihEgor/helix that referenced this pull request Jan 4, 2025
diucicd pushed a commit to diucicd/helix that referenced this pull request Jan 8, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants